SSE and Streaming Support POC #1422
Conversation
…TOKEN for HF domains (or local for testing)
|
ah btw i've "approved" for the CI to get run |
packages/mcp-client/src/McpClient.ts
Outdated
| @@ -23,7 +29,7 @@ export interface ChatCompletionInputMessageTool extends ChatCompletionInputMessa | |||
|
|
|||
| export class McpClient { | |||
| protected client: InferenceClient; | |||
| protected provider: InferenceProviderOrPolicy | undefined; | |||
| protected provider: InferenceProvider | undefined; | |||
There was a problem hiding this comment.
you'll need to merge main as this was changed (very slightly) on main
There was a problem hiding this comment.
(meaning InferenceProviderOrPolicy is the new type – though i agree it's a mouthful) ie. you can just revert those changes here and below
There was a problem hiding this comment.
basically what this does is we now accept some "virtual" providers like "auto" (which points to your preferred provider in your HF settings that serve the model in question)
cc @Wauplin @hanouticelina from our client SDK team
packages/mcp-client/package.json
Outdated
| @@ -55,6 +55,7 @@ | |||
| "dependencies": { | |||
| "@huggingface/inference": "workspace:^", | |||
| "@huggingface/tasks": "workspace:^", | |||
| "@modelcontextprotocol/sdk": "^1.9.0" | |||
| "@modelcontextprotocol/sdk": ">=1.11.2", | |||
There was a problem hiding this comment.
| "@modelcontextprotocol/sdk": ">=1.11.2", | |
| "@modelcontextprotocol/sdk": "^1.11.2", |
better to "pin" an exact dependency imo
packages/mcp-client/src/Agent.ts
Outdated
| @@ -44,7 +45,7 @@ const askQuestionTool: ChatCompletionInputTool = { | |||
| const exitLoopTools = [taskCompletionTool, askQuestionTool]; | |||
|
|
|||
| export class Agent extends McpClient { | |||
| private readonly servers: StdioServerParameters[]; | |||
| private readonly servers: (ServerConfig|StdioServerParameters)[]; | |||
There was a problem hiding this comment.
if you're using VS Code, it should pick up the formatting config automatically, otherwise:
pnpm lint and pnpm format
packages/mcp-client/cli.ts
Outdated
| // Handle --url parameter: parse comma-separated URLs into ServerConfig objects | ||
| const urlIndex = process.argv.indexOf("--url"); | ||
| if (urlIndex !== -1 && urlIndex + 1 < process.argv.length) { | ||
| const urlsArg = process.argv[urlIndex + 1]; | ||
| const urls = urlsArg | ||
| .split(",") | ||
| .map((url) => url.trim()) | ||
| .filter((url) => url.length > 0); | ||
|
|
||
| for (const url of urls) { | ||
| try { | ||
| SERVERS.push(urlToServerConfig(url)); | ||
| } catch (error) { | ||
| console.error(`Error adding server from URL "${url}": ${error.message}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
i think you can just use this:
There was a problem hiding this comment.
and i would still keep the ones above as default (the stdio ones) if nothing is passed
There was a problem hiding this comment.
Longer term I've been thinking of splitting this into a @huggingface/tiny-agents subpackage, where contributors could be interested in pushing combination of MCP servers + models + providers, WDYT?
There was a problem hiding this comment.
pushed here 8d287af , wanted to open a PR against your PR but a bit of a pain given it's on a fork (will give you Write rights to this repo @evalstate )
There was a problem hiding this comment.
Longer term I've been thinking of splitting this into a @huggingface/tiny-agents subpackage, where contributors could be interested in pushing combination of MCP servers + models + providers, WDYT?
like the idea, would be good to get collections of useful agents in one place. should probably iterate a couple more times to settle the config etc.
packages/mcp-client/package.json
Outdated
| @@ -55,6 +55,7 @@ | |||
| "dependencies": { | |||
| "@huggingface/inference": "workspace:^", | |||
| "@huggingface/tasks": "workspace:^", | |||
| "@modelcontextprotocol/sdk": "^1.9.0" | |||
| "@modelcontextprotocol/sdk": "^1.11.2", | |||
| "vitest": "^3.1.2" | |||
There was a problem hiding this comment.
| "vitest": "^3.1.2" |
it's already a dependency at the root-level, did it not work as it was? cc @coyotte508
packages/mcp-client/src/utils.ts
Outdated
| if (path.endsWith("/sse")) { | ||
| type = "sse"; | ||
| } else if (path.endsWith("/mcp")) { | ||
| type = "streamableHttp"; |
There was a problem hiding this comment.
wow this is actually enforced by the spec that the URL needs to end with /mcp? quite a big convention push, but fine i guess
packages/mcp-client/src/types.ts
Outdated
| export type ServerConfig = | ||
| | { type: "stdio"; config: StdioServerParameters } | ||
| | { type: "sse"; config: SSEServerConfig } | ||
| | { type: "streamableHttp"; config: StreamableHTTPServerConfig }; |
There was a problem hiding this comment.
| | { type: "streamableHttp"; config: StreamableHTTPServerConfig }; | |
| | { type: "http"; config: StreamableHTTPServerConfig }; |
i think i would just rename this to "http"
packages/mcp-client/pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
(need to push the resolved .lock too)
packages/mcp-client/src/McpClient.ts
Outdated
| @@ -170,7 +198,7 @@ export class McpClient { | |||
| const client = this.clients.get(toolName); | |||
| if (client) { | |||
| const result = await client.callTool({ name: toolName, arguments: toolArgs, signal: opts.abortSignal }); | |||
| toolMessage.content = (result.content as Array<{ text: string }>)[0].text; | |||
| toolMessage.content = ResultFormatter.format(result as CallToolResult); | |||
There was a problem hiding this comment.
(nit) i take this as a "bug" in the TS MCP SDK that we need to cast this as a CallToolResult, no?
There was a problem hiding this comment.
marked as ready for review, give it a final look @evalstate and then we can merge it?
Co-authored-by: Julien Chaumond <[email protected]>
Co-authored-by: Julien Chaumond <[email protected]>
Update to enable:
pnpm agent --url https://<host>/mcp. You can specify multiple endpoints with--url <url1> --url <url2>